-
Notifications
You must be signed in to change notification settings - Fork 39
[양지선] sprint4 #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[양지선] sprint4 #150
The head ref may contain hidden characters: "Basic-\uC591\uC9C0\uC120-sprint4"
Conversation
GANGYIKIM
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지선님 4번째 미션 제출 수고하셨습니다.
기본 요구사항을 잘 작성하셔서 심화도 금방 하실 것 같아요!
시간이 되실때 심화도 구현해보시면 좋겠습니다~
다음 React 미션도 화이팅이에요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
이러한 vscode 설정 파일은 github에 올라가지 않게 해주시는 것을 추천드려요.
github는 프로젝트를 공유하기 위한 공간으로 해당 프로젝트와 관련된 파일들만 올라가는 것이 좋습니다.
어떤 사람은 vscode를 쓰지 않고 다른 에디터를 쓸 수 있고 쓰더라도 port 번호를 다르게 할 수 있습니다.
따라서 이런 파일들은 변경사항에서 의도적으로 제외하시고 올리시거나 gitignore 같은 파일을 생성하셔서
자동으로 변경사항이 잡히지 않도록 하시는 것을 추천드립니다~
| </div> | ||
| </div> | ||
| </footer> | ||
| <script src="validation.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
아마 script 태그를 HTML의 하단에 배치하신 이유가 script가 문서의 렌더링을 막지 않도록 하기 위해서이실 것 같아요.
하지만, script 태그에 defer나 async 속성을 사용하면 이런 문제를 해결할 수 있기 때문에 반드시 하단에 배치할 필요는 없습니다!
또한 script 태그는 상단에 있는게 구조 파악에서도 유리하기 때문에 상단 head 태그에 두시는 것을 추천드려요~
지금과 같은 경우 DOM을 조작하는 JS 이니 defer 속성을 추가하시면 되겠습니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
한 파일에서 로그인, 회원가입 공용으로 JS를 작성해주셨네요~
둘이 공통되는 부분이 많이 이렇게 해주신 것 같아요.
다만 그렇게 되면 코드가 복잡해져서 유지보수성이 떨어지고 로그인 페이지의 경우 불필요한 코드까지 알아야합니다~
가능하면 공통 로직들은 분리해주시고 각 페이지별 JS를 따로 만드시는 것을 추천드려요!
| @@ -0,0 +1,148 @@ | |||
| console.log('validation.js loaded check'); | |||
| document.addEventListener('DOMContentLoaded', function () { | |||
| const emailLogin = document.getElementById('email-login'); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❗️ 수정요청
변수명은 가독성에 있어서 중요한 역할을 합니다. 따라서 어떤 값인지 알 수 있는 변수명을 추천드려요.
아마 로그인과 회원가입에서 같은 js 파일을 쓰시느라 이렇게 이름을 지으신 것 같은데 다른 코멘트에서 말씀드린 것처럼 파일을 분리하시고 변수명은 email-login이라는 클래스명을 가진 input 태그이니 emailInput 과 같은 이름으로 변경하시는 것을 추천드립니다.
| const emailLogin = document.getElementById('email-login'); | |
| const emailInput = document.getElementById('email-login'); |
| function validateEmail(email) { | ||
| const regex = /^[^\s@]+@[^\s@]+.[^\s@]+$/; | ||
|
|
||
| return regex.test(email); | ||
| } | ||
|
|
||
| function validatePassword(password) { | ||
| return password.length >= 8; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 칭찬
유효성 검사 따로 함수로 빼신 것 좋습니다.
| const isValidSignup = | ||
| emailSignup?.value && | ||
| nicknameSignup?.value && | ||
| passwordSignup?.value && | ||
| passwordConfirmationSignup?.value && | ||
| validateEmail(emailSignup.value) && | ||
| validatePassword(passwordSignup.value) && | ||
| passwordSignup.value === passwordConfirmationSignup.value; | ||
| if (signupButton) signupButton.disabled = !isValidSignup; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
이런경우 가독성을 위해 따로 함수로 빼면 좋을 것 같아요.
| const isValidSignup = | |
| emailSignup?.value && | |
| nicknameSignup?.value && | |
| passwordSignup?.value && | |
| passwordConfirmationSignup?.value && | |
| validateEmail(emailSignup.value) && | |
| validatePassword(passwordSignup.value) && | |
| passwordSignup.value === passwordConfirmationSignup.value; | |
| if (signupButton) signupButton.disabled = !isValidSignup; | |
| } | |
| // 로그인, 회원가입 로직 분리시 코드 | |
| signupButton.disabled = !checkFormValid(); | |
| } |
| input.classList.add('error'); | ||
| const errorMessage = input.closest('.form-group').querySelector('.error-message'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
자바스크립트에서 이런 변수들은 호이스팅됩니다. 따라서 가능한 변수, 함수선언문들은 해당 코드블록 최상단에 두시는 것이 좋습니다.
| input.classList.add('error'); | |
| const errorMessage = input.closest('.form-group').querySelector('.error-message'); | |
| const errorMessage = input.closest('.form-group').querySelector('.error-message'); | |
| input.classList.add('error'); |
| if (passwordSignup) { | ||
| passwordSignup.addEventListener('focusout', function () { | ||
| if (!passwordSignup.value) { | ||
| showError(passwordSignup, '비밀번호를 입력해주세요.'); | ||
| } else if (!validatePassword(passwordSignup.value)) { | ||
| showError(passwordSignup, '비밀번호를 8자 이상 입력해주세요.'); | ||
| } else { | ||
| clearError(passwordSignup); | ||
| } | ||
| toggleButtonState(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
지금 배포된 사이트에서 테스트를 해보면, 비밀번호와 비밀번호 확인 인풋의 값이 같았다가 비밀번호 인풋을 변경하면 에러가 나지 않으며 버튼이 활성화되지 않도록 구현되어 있습니다.
유저 입장에서는 피드백없이 진행이 불가능해지는 것이므로 의도한 동작이 아니라면 확인해보시고 수정하시면 좋겠습니다~
요구사항
기본
로그인
회원가입
심화
배포
멘토에게